Skip to content

Implemented AsyncInMemoryDatabase::set in terms of ::batch_set#425

Closed
cryo28 wants to merge 1 commit intofacebook:mainfrom
cryo28:main
Closed

Implemented AsyncInMemoryDatabase::set in terms of ::batch_set#425
cryo28 wants to merge 1 commit intofacebook:mainfrom
cryo28:main

Conversation

@cryo28
Copy link
Contributor

@cryo28 cryo28 commented Mar 7, 2024

The implementation of AsyncInMemoryDatabase::set and batch_set contain a lot of duplicate code.

Tested with:

cargo test test_in_memory_db

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.43%. Comparing base (24b11de) to head (d759c17).

Files Patch % Lines
akd/src/storage/memory.rs 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   87.99%   88.43%   +0.43%     
==========================================
  Files          39       39              
  Lines        9054     9198     +144     
==========================================
+ Hits         7967     8134     +167     
+ Misses       1087     1064      -23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kevinlewi
kevinlewi approved these changes Mar 10, 2024
View reviewed changes
Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks and LGTM. However, seems like there is a clippy error that is on a line which is unrelated to your changes, but if you can fix it so that CI tests pass, that would be great!

https://github.com/facebook/akd/actions/runs/8195091568/job/22412621676?pr=425#step:4:556

@kevinlewi
Copy link
Contributor

kevinlewi commented Mar 15, 2024

Sorry, I landed #426 first which also fixed the clippy error -- can you rebase on main and force-push again? Should fix the CI issue

@cryo28
Copy link
Contributor Author

cryo28 commented Mar 16, 2024

It seems like force updates don't re-trigger workflows. Not sure what I should do? Should I recreate the PR or add some dummy commit to the existing one?

@cryo28 cryo28 closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinlewi kevinlewi kevinlewi approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cryo28 @codecov-commenter @kevinlewi @facebook-github-bot